In bug 767640, I made Ci, Cr, Cc, and Cu to be defined on chrome scopes. I also removed many definitions of these variables from .jsms. I only removed the simplest form, which is stuff like const Cc = Components.classes; Specifically, I used the regexp: ^(const|let|var)\s+(Cc|Ci|Cr|Cu)\s*=\s*Components.(classes|interfaces|results|utils)\s*;\s*$ So, stuff needs to be removed from files that don't end in .jsm. The simple form also needs to be removed when it starts with whitespace. I was having problems with that, but the version of my patch that I was using was broken so maybe it will actually work now. Another common form of definitions that should be removed look like this: const {classes: Cc, utils: Cu, interfaces: Ci} = Components;

My script in bug 767640 could probably be modified to handle these extra cases. There may be other cases I haven't come across.

I guess I'll take a crack at this...

A few places use Cm. Maybe I should add that first just to make things a little cleaner.

(In reply to Andrew McCreight [:mccr8] from comment #3) > A few places use Cm. Maybe I should add that first just to make things a > little cleaner. There are also a few uses of CC (Components.Constructor) https://searchfox.org/mozilla-central/search?q=+CC%28&case=true&path=.js

The set of file extensions that matched my regexes is: js, jsm, html, py, sjs, xhtml, xul Additionally, there's is an instance of it in testing/marionette/doc/CodeStyle.md which I guess needs to be updated at some point.

This patch is entirely generated by my script. - It covers every file with extension js, jsm, html, py, sjs, xhtml, xul. - It removes blank lines after removed lines, when the removed lines are preceded by either blank lines or the start of a new block. The "start of a new block" is defined fairly hackily: either the line starts with //, ends with */, ends with {, """ or '''. The first two cover comments, the third one covers JS, and the final two cover JS embedded in Python. This also applies if the removed line was the first line of the file. - It covers the pattern matching cases like "var {classes: Cc, interfaces: Ci, utils: Cu, results: Cr} = Components;". It'll remove the entire thing if they are all either Ci, Cr, Cc or Cu, or it will remove the appropriate ones and leave the residue behind. If there's only one behind, then it will turn it into a normal, non-pattern matching variable definition. (For instance, "const { classes: Cc, Constructor: CC, interfaces: Ci, utils: Cu } = Components" becomes "const CC = Components.Constructor".) This mostly passes eslint, but there are a few places where it complains about an empty block. I'll fix those by hand in a separate patch. I did a try run on an earlier version of the patch and there were some test failures I need to investigate. https://treeherder.mozilla.org/#/jobs?repo=try&revision=4908edeb447efad48e0c6c139644de2b0e59b4b4

The updated version of my autogenerated patch has: 1919 files changed, 56 insertions(+), 4325 deletions(-)

I had to blacklist test_bug790732.html and dbg-actors.js. The former because it is defining Ci in content, so the behavior isn't affected by my patch. I'm not sure why the latter doesn't work. There must be something weird about the way it gets loaded (as part of some XPCShell thing, via the devtools loader stuff), but I don't know what.

opt try run, including Talos: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b26403a1bfd0340e8875486a5d3328f24b19c649 (debug had previously only shown the same problems as opt builds)

Comment on attachment 8947903 [details] Bug 1432992, part 2 - Manually remove some empty blocks. https://reviewboard.mozilla.org/r/217580/#review223602 There's a similar empty block in these 3 files: testing/mochitest/browser-harness.xul testing/mochitest/harness.xul dom/events/test/test_bug415498.xul

Comment on attachment 8947904 [details] Bug 1432992, part 3 - Adjust some line numbers in tests. https://reviewboard.mozilla.org/r/217582/#review223606 rs=me if try is happy.

Comment on attachment 8947902 [details] Bug 1432992, part 1 - Remove definitions of Ci, Cr, Cc, and Cu. https://reviewboard.mozilla.org/r/217578/#review223588 This looks good, thanks for doing it! I reviewed browser/base/content, toolkit/components/search, toolkit/content/, all the files with modified lines rather than just plain removals, all the head*.js files, and a few random files here and there. Comments: - tools/code-coverage/tests/xpcshell/head.js now contains only a license header, and netwerk/test/unit_ipc/head_cc.js is now empty. Please remove these 2 files as part of a manual cleanup. - some files now have a CDATA block that starts with an empty line, when it wasn't the case before. Affected files: browser/base/content/test/chrome/test_aboutCrashed.xul dom/base/test/chrome/file_bug1139964.xul dom/base/test/chrome/file_bug549682.xul dom/base/test/chrome/test_permission_isHandlingUserInput.xul editor/libeditor/tests/test_bug1397412.xul js/xpconnect/tests/chrome/test_APIExposer.xul toolkit/content/tests/widgets/test_popupreflows.xul toolkit/mozapps/downloads/tests/chrome/test_unknownContentType_delayedbutton.xul toolkit/mozapps/downloads/tests/chrome/test_unknownContentType_dialog_layout.xul widget/tests/test_system_status_bar.xul This is the only correctness issue I've found in the output of your script. Given that it only affects test files and that other test files already had this pattern, I'm not requiring a new version of the script or a cleanup here. I'll leave it up to you to decide if you want to cleanup these 10 files by hand. - browser/components/places/content/placesOverlay.xul has a comment that should be removed (but you can ask Marco to do it for you in bug 406371).

(In reply to Florian Quèze [:florian] from comment #17) > There's a similar empty block in these 3 files: Fixed.

(In reply to Florian Quèze [:florian] from comment #19) > This looks good, thanks for doing it! Thanks for the review! > - tools/code-coverage/tests/xpcshell/head.js now contains only a license > header, and netwerk/test/unit_ipc/head_cc.js is now empty. Please remove > these 2 files as part of a manual cleanup. Fixed, in part 2. > - some files now have a CDATA block that starts with an empty line, when it > wasn't the case before. Good catch. I added a CDATA case to my script's code for removing blank lines, and made sure that every file you mentioned was fixed. There were two additional instances of this issue not in your list (docshell/test/chrome/test_bug789773.xul and js/xpconnect/tests/chrome/test_cows.xul). > - browser/components/places/content/placesOverlay.xul has a comment that > should be removed (but you can ask Marco to do it for you in bug 406371). Fixed in part 2. I'm going to do a full try run to make sure there aren't any lingering issues I failed to notice so far.

(I'm going to wait to push my updated version of the patch to the bug until I'm ready to land it, to avoid churning on this huge patch, but I can upload it sooner if you prefer.)

Unfortunately, my patches have a number of problems on Android. The biggest one is that httpd.js is run from xpcshell to run reftests, and for some reason Cu is not defined, so reftests and crash tests all fail. There are also three Mochitests that seem to fail due to my patches. I don't know why the failures are Android-only. https://treeherder.mozilla.org/#/jobs?repo=try&revision=7a9a215209cfd9076c0762a754dcae3f02ee3e80 (the XPCShell failures present on all platforms are not related to my patch)

(In reply to Andrew McCreight [:mccr8] from comment #24) > Unfortunately, my patches have a number of problems on Android. The biggest > one is that httpd.js is run from xpcshell to run reftests, and for some > reason Cu is not defined, so reftests and crash tests all fail. There are > also three Mochitests that seem to fail due to my patches. I don't know why > the failures are Android-only. > https://treeherder.mozilla.org/#/ > jobs?repo=try&revision=7a9a215209cfd9076c0762a754dcae3f02ee3e80 > (the XPCShell failures present on all platforms are not related to my patch) Could this be the same hostutils stuff that plagued kmag's patches in bug 1431533? I didn't follow closely, but off-hand it sounds like it could be similar.

(In reply to :Gijs from comment #25) > the same hostutils stuff x-ref bug 1433279.

(In reply to :Gijs from comment #25) > Could this be the same hostutils stuff that plagued kmag's patches in bug > 1431533? I didn't follow closely, but off-hand it sounds like it could be > similar. Oh, it sounds like the version of XPCShell used to run the server code on Android is different than the actual version of the tree? That's surprising. Thanks for pointing that out, as I would never have figured that out on my own. Maybe I'll split off the httpd.js and *.sjs changes into a separate bug, and hope there's nothing else in the 2000 files I'm changing that runs in the old version.

(In reply to Andrew McCreight [:mccr8] from comment #27) > Maybe I'll split off the httpd.js and *.sjs changes into a separate > bug, and hope there's nothing else in the 2000 files I'm changing that runs > in the old version. Unfortunately it's not that simple. I almost tried that myself, but the mochitest server pulls in a bunch of JSMs, starting with NetUtil.jsm, XPCOMUtils.jsm, Services.jsm, ...

(In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're blocked) from comment #28) > Unfortunately it's not that simple. I almost tried that myself, but the > mochitest server pulls in a bunch of JSMs, starting with NetUtil.jsm, > XPCOMUtils.jsm, Services.jsm, ... Mochitest itself was fine, for whatever reason. The problem was reftests and a few SJS files. https://treeherder.mozilla.org/#/jobs?repo=try&revision=a677790fb3f41330678008cae7c7028ee10adfa5 Of course, test_bootstrap.js seems to be failing now. At least that's on Linux where I can investigate it more easily.

I couldn't repro the test_bootstrap.js failure locally, not even against the same revision, and when I rebased it, it seems to have gone away, so I'm going to hope it was a fluke or something that got fixed in the mean time... https://treeherder.mozilla.org/#/jobs?repo=try&revision=21b551ff59001fbca8d1a17c26bb23737a12e22c

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again. hg error in cmd: hg rebase -s 6c752e9f4403aac6276c1e837b7d561e86e6a572 -d 828f63e2d52a: rebasing 446027:6c752e9f4403 "Bug 1432992, part 1 - Remove definitions of Ci, Cr, Cc, and Cu. r=florian" merging browser/base/content/browser.js merging browser/components/nsBrowserGlue.js merging browser/components/places/PlacesUIUtils.jsm merging browser/components/preferences/SiteDataManager.jsm merging browser/components/preferences/in-content/preferences.js merging browser/components/preferences/in-content/tests/browser_clearSiteData.js and browser/components/preferences/in-content/tests/browser_siteData.js to browser/components/preferences/in-content/tests/browser_clearSiteData.js merging browser/components/preferences/in-content/tests/browser_siteData.js merging browser/components/preferences/siteDataSettings.js merging browser/extensions/webcompat-reporter/content/WebCompatReporter.jsm merging toolkit/components/places/PlacesUtils.jsm merging toolkit/components/places/SyncedBookmarksMirror.jsm merging toolkit/components/url-classifier/SafeBrowsing.jsm merging toolkit/content/aboutTelemetry.js warning: conflicts while merging browser/components/preferences/in-content/preferences.js! (edit, then use 'hg resolve --mark') warning: conflicts while merging browser/components/preferences/in-content/tests/browser_clearSiteData.js! (edit, then use 'hg resolve --mark') warning: conflicts while merging browser/components/preferences/in-content/tests/browser_siteData.js! (edit, then use 'hg resolve --mark') unresolved conflicts (see hg resolve, then hg rebase --continue)

I guess I'll probably have to land this myself on inbound.

Pushed by amccreight@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/b38d59f71915 part 1 - Remove definitions of Ci, Cr, Cc, and Cu. r=florian https://hg.mozilla.org/integration/autoland/rev/f06620ef048d part 2 - Manually remove some empty blocks. r=florian https://hg.mozilla.org/integration/autoland/rev/f033d62a90ad part 3 - Adjust some line numbers in tests. r=florian

https://hg.mozilla.org/mozilla-central/rev/b38d59f71915 https://hg.mozilla.org/mozilla-central/rev/f06620ef048d https://hg.mozilla.org/mozilla-central/rev/f033d62a90ad

Do we have eslint rules to prevent re-introducing them?

(In reply to Masatoshi Kimura [:emk] from comment #43) > Do we have eslint rules to prevent re-introducing them? Please see the blocking list. There's a couple of bugs to tidy up on the ESLint front.

Commit pushed to master at https://github.com/mozilla/activity-stream https://github.com/mozilla/activity-stream/commit/1737e173d3549a58468370fdfd75e1eabc22194c chore(mc): Port Bug 1432992 - Remove definitions of Ci, Cr, Cc, and Cu